Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new static-index validation can still be bypassed via implicit narrowing/truncation of decoded indexes (uint64_t → uint16_t), so invalid peer-supplied indexes may not be reliably rejected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR strengthens HTTP/3 QPACK robustness by rejecting invalid peer-supplied static table indexes and by treating failed encoder-stream name-reference lookups as fatal before attempting dynamic table insertion.
Changes:
- Add bounds-checking to QPACK static table lookups to prevent out-of-range access.
- Abort decoding when an encoder-stream “Insert With Name Reference” lookup does not resolve to an EXACT match (static or dynamic).
- Add a unit test asserting that decoding fails for an out-of-range static table index.
File summaries
| File | Description |
|---|---|
| src/proxy/http3/QPACK.cc | Adds static-table bounds checking and enforces lookup success for encoder-stream insert-with-name-ref before inserting into the dynamic table. |
| src/proxy/http3/test/test_QPACK.cc | Adds a decode-failure test case for an out-of-range static-table index. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
The clang-analyzer job's Clang-Tidy stage failure here is not from this change. The only diagnostic is pre-existing on master: That is fixed by #13622. I will rebase this branch once that merges. |
|
[approve ci clang-analyzer] |
7a84b8b to
87c9323
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There is a concrete error-path handling bug in _read_insert_with_name_ref() that can fall through on decode failure (risking incorrect buffer consumption), and the new polling-based tests introduce a cross-thread data race without atomic/mutex protection.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
src/proxy/http3/QPACK.cc:1542
- The value decode error check can fall through on xpack_decode_string() failure:
tmpis uninitialized on error, and if the condition is false the code continues with a negativeret, which can wrapread_len(size_t) and consume the wrong amount of data. Treat anyret < 0from xpack_decode_string() as a hard failure before updatingread_len.
// Value
if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0 &&
tmp > 0xFF) {
return -1;
}
src/proxy/http3/test/test_QPACK.cc:116
TestQPACKEventHandler::_eventis written from an event thread and read from the test thread (via wait_for_event polling). As a plain int this is a data race (UB) and can make the new polling helper flaky under TSAN or on weakly-ordered CPUs. Make_eventatomic (or protect access with a mutex) and use load/store in the handler and accessor.
TestQPACKEventHandler() : Continuation() { SET_HANDLER(&TestQPACKEventHandler::event_handler); }
int
event_handler(int event, Event * /* data ATS_UNUSED */)
{
this->_event = event;
return 0;
}
int
last_event()
{
return this->_event;
}
private:
int _event = 0;
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
87c9323 to
541fc9e
Compare
|
The rocky ASan job's LeakSanitizer failure is fixed in the force push just now. Both leaks were reached for the first time by the new encoder stream test:
Both were pre-existing: nothing called Verified with a local ASan build ( |
maskit
left a comment
There was a problem hiding this comment.
Built and ran this against quiche + ASan (test_qpack): compiles clean, all three [qpack-decode] cases pass in 1.4s. I reverted each fix separately to confirm the new tests bite — without the countof check test 1 aborts, without the encoder-stream handling test 2 fails both assertions. STATIC_HEADER_FIELDS also matches RFC 9204 Appendix A entry for entry, so countof is the right bound.
Two blockers inline, plus a note on the new test's threading.
Validate peer-supplied static table indexes before reading the QPACK static table. Also honor failed encoder-stream name-reference lookups before inserting dynamic table entries, and resolve a dynamic table name reference there relatively, as RFC 9204 specifies for the encoder stream. XpackDynamicTable::lookup_relative() counted back from the head entry without checking that there is one, so an empty table read past the end of its entry allocation. Bound the relative index by count() instead. The new encoder stream test is the first to exercise QPACK::on_stream_open, which allocated a QUICStreamVCAdapter::IOInfo that nothing owned or freed. Hold it in a per-stream map and erase it in on_stream_close, the way the other QUICApplication implementations do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
541fc9e to
3b02b7e
Compare
| this->_abort_decode(); | ||
| return EVENT_DONE; | ||
| } | ||
| this->_dynamic_table.insert_entry(name, name_len, value, value_len); |

Validate peer-supplied static table indexes before reading the QPACK
static table. Also honor failed encoder-stream name-reference lookups
before inserting dynamic table entries, and resolve a dynamic table
name reference there relatively, as RFC 9204 specifies for the encoder
stream.
XpackDynamicTable::lookup_relative() counted back from the head entry
without checking that there is one, so an empty table read past the end
of its entry allocation. Bound the relative index by count() instead.
The new encoder stream test is the first to exercise
QPACK::on_stream_open, which allocated a QUICStreamVCAdapter::IOInfo
that nothing owned or freed. Hold it in a per-stream map and erase it in
on_stream_close, the way the other QUICApplication implementations do.